Conversation
|
Hmm, all tests passed on my end 🤔 |
| schema: | ||
| type: integer | ||
| example: 1 | ||
| example: 42 |
There was a problem hiding this comment.
Note) 42 came from the updated test
|
@k0kubun Could you take a look 🙇 |
|
Sorry, I didn't read this when the CI was not passing and I didn't get a notification when it started to pass. I'll take a look. |
| def build_parameters(record) | ||
| parameters = [] | ||
|
|
||
| marker_to_keep_last_duplicate = Time.now.utc.round.to_s |
There was a problem hiding this comment.
The PR is now concise enough to make me understand how and why it's used. Thank you.
I have some suggestions here though:
- Can we insert them inside
SchemaMergerinstead ofSchemaBuilder? This seems to be the concern forSchemaMergerand it feels like the current implementation scatters the implementation details to unrelated places. This PR should touchSchemaBuilder. - Could we achieve the same thing by just always prepending new schema elements to the array in
SchemaMergerand callinguniq!after that? It should give the same result as sorting based on__marker.
There was a problem hiding this comment.
Can we insert them inside SchemaMerger instead of SchemaBuilder?
It makes sense. 6326392
And it turned out that __marker is not needed. 1128a0c
Could we achieve the same thing by just always prepending new schema elements to the array in SchemaMerger and calling uniq! after that?
Yes, sort! removed 2e88f76
Without sort!, diffs on the existing rails/doc/openapi.(json|yml) is bigger.
This is a spin-out from #57
After this PR,
parametersarray preserves the occurrence with newer value inexampleCurrent implementation
It keeps the first occurrence of duplicated items with same
name&in.If user update an existing parameter in their rspec, a new parameter object with newer value in
examplefieldis added toparameters` array ... but is removed soon because it is not the first occurrence.In other words, the occurrence with oldest example is kept forever.
Problems
"Preserving first occurrence" will cause a problem for some cases.
1. User changed the value format.
Let's say user changed the format of
idfrom numeric (123) to ulid (01ARZ3NDEKTSV4RRFFQ69G5FAV).[{ name: "id", in: "path", example: "123" }][{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"01ARZ3NDEKTSV4RRFFQ69G5FAV" }][{ name: "id", in: "path", example: "123" }]😭 The preserved
123is wrong about format.2. rspec-openapi support another field of parameter object
Let's say rspec-openapi added
descriptionfield in parameter object somehow.[{ name: "id", in: "path", example: "123" }][{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"123", description:"blah blah" }][{ name: "id", in: "path", example: "123" }]😭
descriptionis not added.Solution
1. Keep a last occurrence
Unfortunately, it brings unstable order.
Let's say we invokes
/foo/123and/foo/456in rspec.[{ name: "id", in: "path", example: "123" }][{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"456" }][{ name: "id", in: "path", example: "456" }][{ name: "id", in: "path", example: "456" }][{ name: "id", in: "path", example: "456" }, { name: "id", in: "path", example: "123" } ][{ name: "id", in: "path", example: "123" }]😭
123and456is oscillating for each run.2. Introduces a timestamp marker to be used as sort key
I think it is better to introduce
__marker: Timestampfield toparameters.Then, merger sorts the parameters and preserves the duplicates with largest
__marker, so the older occurrencenes will be removed.This PR also sorts parameters by
inandnamebefore__marker, becausesort_byis not "stable sort"